Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add catch for ConnectionAborted in Http3 #93049

Merged
merged 3 commits into from
Oct 6, 2023
Merged

Add catch for ConnectionAborted in Http3 #93049

merged 3 commits into from
Oct 6, 2023

Conversation

liveans
Copy link
Member

@liveans liveans commented Oct 5, 2023 •
edited
Loading

It wasn't actually a race between, closing, etc. Since we're keeping the first exception in _abortException, we're returning that and in some cases, we weren't actually wrapping ConnectionAbort with HttpProtocolException, and it was resulting in throwing a QuicException.

Fixes #92359

@liveans liveans requested review from rzikm and a team October 5, 2023 06:57
@ghost ghost assigned liveans Oct 5, 2023
@ghost
Copy link

ghost commented Oct 5, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

I don't have a good repro but according to the stack trace, it feels like abort due to dispose and close was racing.

Fixes #92359

Author: liveans
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@ManickaP
Copy link
Member

ManickaP commented Oct 5, 2023

it feels like abort due to dispose and close was racing.

Could you expand on that a bit more so I can fully understand the sequence of events that could cause this?

@liveans
Copy link
Member Author

liveans commented Oct 5, 2023 •
edited
Loading

it feels like abort due to dispose and close was racing.

Could you expand on that a bit more so I can fully understand the sequence of events that could cause this?

AFAIU, when we finished the serverTask Http3Connection was disposing faster than before we sent Close, so it caused QuicConnection to dispose of, and disposing of QuicConnection was causing connection aborted by peer.

@build-analysis build-analysis bot mentioned this pull request Oct 5, 2023
Closed
@ManickaP
Copy link
Member

ManickaP commented Oct 5, 2023

Http3Connection was disposing faster than before we sent Close

CloseAsync is awaited, so we're waiting for the SHUTDOWN_COMPLETE event for the connection

disposing of QuicConnection was causing connection aborted by peer.

Dispose won't do anything if close was already called, if not, it will do the same thing as CloseAsync called with default connection close error code. Also "connection aborted by peer" is what you will get when you call CloseAsync so that should be expected:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs

Lines 445 to 451 in ffa4ef7

catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted)
{
Debug.Assert(ex.ApplicationErrorCode.HasValue);
Http3ErrorCode code = (Http3ErrorCode)ex.ApplicationErrorCode.Value;
Abort(HttpProtocolException.CreateHttp3ConnectionException(code, SR.net_http_http3_connection_close));
}

So I think your premise is not correct and there's more to it.

@liveans liveans marked this pull request as draft October 5, 2023 09:33
@liveans liveans marked this pull request as ready for review October 6, 2023 10:35
@liveans liveans requested a review from ManickaP October 6, 2023 10:35
ManickaP
ManickaP approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@liveans liveans changed the title ServerClosesConnection_ThrowsHttpProtocolException Test Race Fix Add catch for ConnectionAborted in Http3 Oct 6, 2023
@liveans liveans merged commit 915a2f9 into dotnet:main Oct 6, 2023
109 checks passed
@liveans liveans added this to the 9.0.0 milestone Oct 6, 2023
@liveans liveans deleted the server_closes_connection_abort_and_close_racing_fix branch October 6, 2023 13:50
@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ManickaP ManickaP ManickaP approved these changes

@rzikm rzikm Awaiting requested review from rzikm

Assignees

@liveans liveans

Labels
area-System.Net.Http
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

ServerClosesConnection_ThrowsHttpProtocolException test failed with wrong exception type
2 participants
@liveans @ManickaP